Use redirect property on HTTP and OpenAPI calls #1013
Conversation
...ttp/src/main/java/io/serverlessworkflow/impl/executors/http/AbstractHttpExecutorBuilder.java
Outdated
Show resolved
Hide resolved
...ttp/src/main/java/io/serverlessworkflow/impl/executors/http/AbstractHttpExecutorBuilder.java
Outdated
Show resolved
Hide resolved
...ttp/src/main/java/io/serverlessworkflow/impl/executors/http/RedirectValidationException.java
Outdated
Show resolved
Hide resolved
...ttp/src/main/java/io/serverlessworkflow/impl/executors/http/AbstractHttpExecutorBuilder.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com>
Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com>
| case HttpMethod.PATCH: | ||
| return new WithBodyRequestSupplier( | ||
| (request, entity) -> request.method("patch", entity), application, body, redirect); |
There was a problem hiding this comment.
We need to discuss about PATCH, I will write some options tomorrow 🛌🏼
There was a problem hiding this comment.
HI folks, I did not have time to solve this one. My new changes:
- Add tests for
DELETE,HEAD,PUTandOPTIONS - Use one method by CNCF Specification file
- Remove for now the support for
PATCH
I removed the support for PATCH just to find a better solution for it, actually the HTTP client lib we are using, uses the java.net.HttpURLConnection that does not support PATCH methods. To achieve it, we have some workarounds:
- To use a property
HttpUrlConnectorProvider.SET_METHOD_WORKAROUNDbut it uses reflection behind the scenes and it is necessary to use--add-opens. (it need an extra configuration) - Use
X-HTTP-Method-Overrideheader (in my opnion, it is a bad idea, it is not guaranteed solution) - Add a new library that supports
PATCHwith no friction.
There was a problem hiding this comment.
@ricardozanini @fjtirado do you have another solution in mind?
There was a problem hiding this comment.
Jersey won't support it, but RestEasy does, for example. The SDK relies on Jersey by default, but downstream clients can override the HttpClient. So it's not always true.
If Jersey, we just fire an UnsupportedOperation exception, or let the HTTP client implementation do so. Document it so users know that the default HTTP client shipped with the SDK does not support PATCH.
There was a problem hiding this comment.
In fact, jersey is only used for testing and explicitly added in examples, it is not really a dependency of the SDK
There was a problem hiding this comment.
@fjtirado but if I use the SDK standalone and once the code reaches Client.newBuilder, it explodes if there's no implementation in the classpath?
|
|
||
| if (!redirect) { | ||
| // disable automatic redirects handling from Jersey client | ||
| request.property("jersey.config.client.followRedirects", false); |
There was a problem hiding this comment.
We can't tie our implementation to this client. Dowstream client code can override the HTTP client factory, hence it won't be Jersey.
We could have a supplier here where the downstream client can be called to react to this property instead.
By default, we set the jersey property or let the supplier from the underlying client do so.
There was a problem hiding this comment.
We can't tie our implementation to this client. Dowstream client code can override the HTTP client factory, hence it won't be Jersey.
Yeah, I forget this spec/impl detail.
We could have a supplier here where the downstream client can be called to react to this property instead.
Do you mean a new method WorkflowApplication.Builder method or a ServiceProvider?
Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com>
Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com>
|
@mcruzdev |
| WorkflowError.communication( | ||
| response.getStatus(), | ||
| task, | ||
| String.format( | ||
| "The property 'redirect' is set to %s but received status %d (%s); expected status in the %s range", | ||
| redirect, | ||
| response.getStatus(), | ||
| response.getStatusInfo().getReasonPhrase(), | ||
| expectedRange)) |
There was a problem hiding this comment.
Actually, lets keep the prevvious code,
Why?, because I feel is more interesting to extract the error message from the response (give potentially a lot of more info to the user) rather than explain why we are throwing the error when redirect is enable, that should be self explanatory by looking at the status code
| boolean isSuccess = statusFamily.equals(SUCCESSFUL); | ||
| boolean isRedirect = statusFamily.equals(REDIRECTION); | ||
| boolean valid = redirect ? (isSuccess || isRedirect) : isSuccess; | ||
|
|
||
| if (!valid) { |
There was a problem hiding this comment.
I think we can simplify the condition, we want to fail if the call was not successful or if redirect flag is false and the status was a redirection one.
| boolean isSuccess = statusFamily.equals(SUCCESSFUL); | |
| boolean isRedirect = statusFamily.equals(REDIRECTION); | |
| boolean valid = redirect ? (isSuccess || isRedirect) : isSuccess; | |
| if (!valid) { | |
| if (statusFamily != SUCCESSFUL || !isRedirect && status == REDIRECTION) { |
3 participants
Changes
RuntimeExceptionthe theserverlessworkflow-impl-httpmodule calledRedirectValidationException.AbstractHttpExecutorBuilderresponsible for applying redirect validation rules for the HTTP response. When the validation fails, it pass the response status code to theWorkflowErrorinstance.TODO
redirectbehavior (when the upstream server returns a 3XX status code)Closes #959